-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Fix wasm table access when accessing functions in splitted module #25432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
We can use the `toIndexType` utility function to access the wasm table
Changes from comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this means we don't have any test coverage for wasm64 + module splitting?
Can you try adding @also_with_wasm64
to test_split_module
? Can you confirm it doesn't work without this fix?
Otherwise lgtm!
src/preamble.js
Outdated
return wasmTable.get(toIndexType(1 + parseInt(prop)))(...args); | ||
#else | ||
return wasmTable.get(toIndexType(prop))(...args); | ||
prop = 1 + parseInt(prop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add a semicolon at end of line.
This seems to be the case. |
lgtm with the test update |
Can you update the test ? |
Sure, do you mean increasing the coverage of the |
While working on split_module + mem64 in my application, I notice more changes required on top of this PR. I'm working on resolving those issues as well |
Yes, I was hoping you can add |
When accessing functions from the splitted module, there's an error because the wasmtable is using table64. So we would have to access it with a BigInt. This has been updated in other part of the codebase, but we missed this file.